Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: do not rewrite absolute paths to avoid long paths #74

Merged
merged 1 commit into from
Oct 16, 2020

Conversation

targos
Copy link
Member

@targos targos commented Oct 16, 2020

This is an alternative to f2c7618.

@MylesBorins @rvagg

@gengjiawen gengjiawen requested a review from rvagg October 16, 2020 06:43
@targos
Copy link
Member Author

targos commented Oct 16, 2020

Tests passed upstream with the changes applied to npm: https://ci.nodejs.org/job/node-test-pull-request/33658/

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean absolute paths still run the risk of breaking the 260 boundary?

Since this gets us green, it's probably better than what we have now, so if you want to cut a gyp-next release, someone can put up a vendor PR to node-gyp and I can cut a release in short order. I'm off for the night but if those things can be done before tomorrow I can move fairly quickly on a release with just this change.

@targos
Copy link
Member Author

targos commented Oct 16, 2020

Does this mean absolute paths still run the risk of breaking the 260 boundary?

No, because skipping this part for absolute paths means that we do not rename them at all, and in this case MSVC just puts the file in the output directory instead of a subdirectory.

@targos targos merged commit c2ccc1a into nodejs:master Oct 16, 2020
@targos targos deleted the fix-windows branch October 16, 2020 11:08
@MylesBorins
Copy link

Are we going to do an update of gyp-next and node-gyp today or should we float this on npm?

MylesBorins pushed a commit to MylesBorins/cli that referenced this pull request Oct 16, 2020
A change to "correctly rename object files for absolute paths"
caused a bug in windows. A fix has already landed upstream in
gyp-next, but has not yet made it's way to a release of gyp-next
or to node-gyp.

Let's float the change until it is fixed upstream.

Refs: nodejs/gyp-next#74
ruyadorno pushed a commit to npm/cli that referenced this pull request Oct 16, 2020
A change to "correctly rename object files for absolute paths"
caused a bug in windows. A fix has already landed upstream in
gyp-next, but has not yet made it's way to a release of gyp-next
or to node-gyp.

Let's float the change until it is fixed upstream.

Refs: nodejs/gyp-next#74

PR-URL: #1974
Credit: @MylesBorins
Close: #1974
Reviewed-by: @ruyadorno
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants